(feat) O3-4201: Enhance Number Question Labels Display Unit and Range (Min/Max) from Concept #454

D-matz · 2024-12-21T22:31:04Z

Add (unit) (min-max) to label, validate input against concept min/max

https://openmrs.atlassian.net/browse/O3-4201

Related: https://openmrs.atlassian.net/browse/O3-4122

4122 also suggests a warning for values within the absolute range but outside normal or critical range. Looking for suggestions on how to implement that, and how to add a test case for a concept with units and min/max

Requirements

This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
My work includes tests or is validated by existing tests.

Summary

Screenshots

Related Issue

Other

…(min-max) to label and min/max to form

D-matz · 2024-12-21T22:55:02Z

@samuelmale @ibacher please let me know if you had other approaches in mind for this or issues

samuelmale

Can you add some test coverage asserting that the units and labels are incorporated in the label?

samuelmale · 2025-01-06T09:41:11Z

src/hooks/useFormFieldsMeta.ts

+        {
+          if(matchingConcept.units)
+          {
+            field.label = field.label + " (" + matchingConcept.units + ")";


This potentially breaks i18n workflows because it mutates the field's label which is implicitly used as the translation key. Since the concept is stored under the field's meta, this can safely be done within the number component post label translation; something like:

<NumberInput id={field.id} label={<FieldLabel field={field} customLabel={t(field.label) + extractFieldUnitsAndRange(field.meta.concept)} />}

@samuelmale thanks, moved the units and range to the number component. Also for a concept that defines min but not max it looks like

leaving critical alone for now

We should harmonize the view here to what we're doing here, which is >= 0 or <= 100 things like that. (Note that "Min" is hard to support because now that's a string that needs to be translated).

unfortunately it splits the label onto a new line, but the format should match the chart lab result format

I mean, that was always going to be a possibility... This just made it apparent for that one case.

samuelmale · 2025-01-06T10:04:36Z

src/hooks/useConcepts.ts

@@ -5,7 +5,7 @@ import { type FetchResponse, type OpenmrsResource, openmrsFetch, restBaseUrl } f
 type ConceptFetchResponse = FetchResponse<{ results: Array<OpenmrsResource> }>;

 const conceptRepresentation =
-  'custom:(uuid,display,conceptClass:(uuid,display),answers:(uuid,display),conceptMappings:(conceptReferenceTerm:(conceptSource:(name),code)))';
+  'custom:(units,lowAbsolute,hiAbsolute,uuid,display,conceptClass:(uuid,display),answers:(uuid,display),conceptMappings:(conceptReferenceTerm:(conceptSource:(name),code)))';


@ibacher when is it necessary to use hiCritical vs lowCritical?

All of the values that aren't lowAbsolute and hiAbsolute are basically only for displaying concerning results (outside the normal range, but inside the critical range) or critical results (outside the critical range, but within the absolute range). Does that make sense? lowAbsolute and hiAbsolute should be rare and only in cases where the value can never exceed that range (e.g., SpO2 is a percent, so it's always between 0 and 100; heights and weights should never be a negative number, things like that).

But we can handle critical icons in a separate PR.

…l, add test concept with units and range

src/hooks/useFormFieldsMeta.ts

samuelmale · 2025-02-26T12:53:40Z

src/components/inputs/number/number.component.tsx

@@ -11,6 +11,28 @@ import { useFormProviderContext } from '../../../provider/form-provider';
 import FieldLabel from '../../field-label/field-label.component';
 import { isEmpty } from '../../../validators/form-validator';

+
+const extractFieldUnitsAndRange = (concept) => {


Can you wrap this in a useCallback?

No, actually, you shouldn't do that here. This is actually done correctly. Functions defined inside functional components would be swapped with a new instance every time the function is called (so, every time the renderer detects that the component might change). useCallback() prevents that by ensuring the variable keeps a stable reference to the same function until the dependency array changes. Since this function isn't defined in a functional component body, references to this function will always be stable references to the same function, so useCallback() isn't needed here (and, IMO, we should prefer these ancillary functions be defined outside of functional components wherever feasible).

All that said, the concept argument here must have a type defined.

Oohh yeah! My bad! For some reason I assumed that the function was defined in the body of the functional component which isn't the case here. It definitely doesn't make sense to memoize such a function.

Should concept come from esm-framework? Or somewhere else or just manually define the interface there? Thanks

ibacher · 2025-02-26T13:37:26Z

src/components/inputs/number/number.component.tsx

@@ -11,6 +11,28 @@ import { useFormProviderContext } from '../../../provider/form-provider';
 import FieldLabel from '../../field-label/field-label.component';
 import { isEmpty } from '../../../validators/form-validator';

+
+const extractFieldUnitsAndRange = (concept) => {


No, actually, you shouldn't do that here. This is actually done correctly. Functions defined inside functional components would be swapped with a new instance every time the function is called (so, every time the renderer detects that the component might change). useCallback() prevents that by ensuring the variable keeps a stable reference to the same function until the dependency array changes. Since this function isn't defined in a functional component body, references to this function will always be stable references to the same function, so useCallback() isn't needed here (and, IMO, we should prefer these ancillary functions be defined outside of functional components wherever feasible).

All that said, the concept argument here must have a type defined.

ibacher · 2025-02-26T13:38:25Z

src/components/inputs/number/number.component.tsx

@@ -61,7 +83,7 @@ const NumberField: React.FC<FormFieldInputProps> = ({ field, value, errors, warn
          id={field.id}
          invalid={errors.length > 0}
          invalidText={errors[0]?.message}
-          label={<FieldLabel field={field} />}
+          label={<FieldLabel field={field} customLabel={t(field.label) + extractFieldUnitsAndRange(field.meta?.concept)} />}


Please don't do string concatenation like this with translated strings. Use i18next's interpolation feature for this.

In this case, the code now hard-codes a word order that doesn't make sense in LTR languages.

I changed the label to t('{{fieldDescription}} {{unitsAndRange}}' but not sure if that's correct as I haven't used i18n before or tested a right to left language case. Thanks for the i18n help.

ibacher · 2025-02-26T13:39:25Z

src/components/inputs/number/number.test.tsx

@@ -22,6 +22,25 @@ const numberFieldMock = {
  readonly: false,
 };

+const numberFieldMockWithUnitsAndRange = {


It would be nice to have a more comprehensive set of tests.

added a few more cases (only units, only range, only max)

Could you add one for lowAbsolute only as well? That should cover things.

ibacher · 2025-02-26T13:39:47Z

src/components/inputs/number/number.component.tsx

+  const hasUpperLimit = hiAbsolute != null;
+
+  if (hasLowerLimit && hasUpperLimit) {
+      return ` (${lowAbsolute} - ${hiAbsolute} ${displayUnit})`;


Don't hard-code prepended spaces in strings here.

…ber-Question-Labels-Display-Unit-and-Range-(Min/Max)-from-Concept

…abel i18n, unit/range test cases

D-matz · 2025-03-23T15:55:56Z

src/components/inputs/number/number.test.tsx

not really sure about jest.mock('react-i18next', () => ({...
it seemed needed for the new i18n label t('{{fieldDescription}} {{unitsAndRange}}'... but I'm not sure if that's correct

ibacher · 2025-03-25T12:42:08Z

src/hooks/useFormFieldsMeta.ts

@@ -11,6 +11,20 @@ export function useFormFieldsMeta(rawFormFields: FormField[], concepts: OpenmrsR
        const matchingConcept = findConceptByReference(field.questionOptions.concept, concepts);
        field.questionOptions.concept = matchingConcept ? matchingConcept.uuid : field.questionOptions.concept;
        field.label = field.label ? field.label : matchingConcept?.display;
+
+        if (matchingConcept) {
+          if (matchingConcept.lowAbsolute != undefined && matchingConcept.hiAbsolute != undefined) {


It's always better to use typeof x !== 'undefined', though this kind of works, but only because SWC will re-write the undefineds here to void 0. In any case, please use exact comparisons (!== and ===).

Also, please just implement @samuelmale's suggestion. It's much cleaner.

applied suggestion @samuelmale ty

ibacher · 2025-03-25T12:44:51Z

src/components/inputs/number/number.component.tsx

+  const hasLowerLimit = lowAbsolute != null;
+  const hasUpperLimit = hiAbsolute != null;


Again !==. But I think here it's preferential to just use isNil() from lodash.

ibacher · 2025-03-25T12:46:30Z

src/components/inputs/number/number.component.tsx

@@ -10,6 +10,28 @@ import { useTranslation } from 'react-i18next';
 import { useFormProviderContext } from '../../../provider/form-provider';
 import FieldLabel from '../../field-label/field-label.component';
 import { isEmpty } from '../../../validators/form-validator';
+import { Concept } from '@openmrs/esm-framework';


Please see our extension documentation on how imports are to be ordered.

moved useTranslation up as well

ibacher · 2025-03-25T12:47:09Z

src/components/inputs/number/number.component.tsx

+            {
+              fieldDescription: t(field.label),
+              unitsAndRange: extractFieldUnitsAndRange(field.meta?.concept),
+              interpolation: { escapeValue: false }


Why is this necessary?

without escapeValue: false, / doesn't show correctly, it becomes / eg BMI (Kg/m2):

ibacher · 2025-03-25T12:49:54Z

src/components/inputs/number/number.component.tsx

@@ -61,7 +83,12 @@ const NumberField: React.FC<FormFieldInputProps> = ({ field, value, errors, warn
          id={field.id}
          invalid={errors.length > 0}
          invalidText={errors[0]?.message}
-          label={<FieldLabel field={field} />}
+          label={<FieldLabel field={field} customLabel={t('{{fieldDescription}} {{unitsAndRange}}',


We should have a real key here with this interpolation as the defaultValue.

thanks again for i18n help, I changed the test too to t: (key, defaultValueOrOptions, options), hopefully that's correct now that there's a key and default value but not sure

ibacher · 2025-03-25T12:50:29Z

src/components/inputs/number/number.test.tsx

+  useTranslation: () => ({
+    t: (key, options) => {
+      if (options && 'fieldDescription' in options) {
+        return `${options.fieldDescription} ${options.unitsAndRange || ''}`.trim();


Since extractFieldUnitsAndRange() always returns a string:

Suggested change

return `${options.fieldDescription} ${options.unitsAndRange || ''}`.trim();

return `${options.fieldDescription} ${options.unitsAndRange}`.trim();

ibacher · 2025-03-25T12:51:26Z

src/components/inputs/number/number.test.tsx

@@ -22,6 +22,25 @@ const numberFieldMock = {
  readonly: false,
 };

+const numberFieldMockWithUnitsAndRange = {


Could you add one for lowAbsolute only as well? That should cover things.

…nly min test, cleaner hi/low from matchingConcept

get units, absolute high, absolute low from concept api, add (units) …

035c318

…(min-max) to label and min/max to form

samuelmale reviewed Jan 6, 2025

View reviewed changes

D-matz added 2 commits January 8, 2025 23:23

display units and min/max from number component instead of field labe…

a8f625a

…l, add test concept with units and range

make unit/range format consistent with lab result

9248fd2

samuelmale reviewed Feb 26, 2025

View reviewed changes

ibacher requested changes Feb 26, 2025

View reviewed changes

D-matz added 2 commits March 8, 2025 12:17

Merge remote-tracking branch 'upstream/main' into O3-4201-Enhance-Num…

da40307

…ber-Question-Labels-Display-Unit-and-Range-(Min/Max)-from-Concept

concept type from esm-framwork, spacing for unit/range cases, field l…

43ee1f0

…abel i18n, unit/range test cases

D-matz commented Mar 23, 2025

View reviewed changes

ibacher requested changes Mar 25, 2025

View reviewed changes

D-matz added 3 commits March 26, 2025 01:07

import isNil and type concept, default for translation and in test, o…

3cc336f

…nly min test, cleaner hi/low from matchingConcept

allow test to work with default value object?

3e9a51c

recommended external modules second

9342a90

		const hasLowerLimit = lowAbsolute != null;
		const hasUpperLimit = hiAbsolute != null;

	return `${options.fieldDescription} ${options.unitsAndRange \|\| ''}`.trim();
	return `${options.fieldDescription} ${options.unitsAndRange}`.trim();

(feat) O3-4201: Enhance Number Question Labels Display Unit and Range (Min/Max) from Concept #454

Are you sure you want to change the base?

(feat) O3-4201: Enhance Number Question Labels Display Unit and Range (Min/Max) from Concept #454

Conversation

D-matz commented Dec 21, 2024 • edited Loading

Requirements

Summary

Screenshots

Related Issue

Other

D-matz commented Dec 21, 2024

samuelmale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibacher Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D-matz Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

D-matz commented Dec 21, 2024 •

edited

Loading

ibacher Jan 9, 2025 •

edited

Loading

D-matz Mar 26, 2025 •

edited

Loading